Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rpm: dkms: Include other kernel-devel packages for spec requirements #16894

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

rpm: dkms: Include other kernel-devel packages for spec requirements #16894

wants to merge 1 commit into from
+3 −3

Conversation

niclimcy
Copy link

@niclimcy niclimcy commented Dec 21, 2024 •
edited
Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

JMOdero
JMOdero reviewed Dec 21, 2024
View reviewed changes
rpm/generic/zfs-dkms.spec.in
Copy link

@JMOdero JMOdero Dec 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure about the use of "and" in lines 33 and 34? That didn't seem to work for me. I deleted those "ands" and just left one blank space between the "or" boolean statements.... Requires: (x or y or z) (a or b or c)

I didn't test the "Conflicts:" line.

@JMOdero
Copy link

JMOdero commented Dec 27, 2024 •
edited
Loading

I haven't had time to test the "Conflicts:" line. As I previously mentioned openzfs compiled and installed fine without it on my system.

However, I'm wondering if something like this would be better if you wish to include it...

Conflicts: (kernel-devel and kernel-longterm-devel) (kernel-devel and kernel-16k-devel) (kernel-longterm-devel and kernel-16k-devel)

As far as "Conflicts:" goes I don't think we need to get into specific version ranges.

AllKind
AllKind suggested changes Dec 27, 2024
View reviewed changes
rpm/generic/zfs-dkms.spec.in
Conflicts: kernel-devel < @ZFS_META_KVER_MIN@, kernel-devel > @[email protected]
Requires: (kernel-devel >= @ZFS_META_KVER_MIN@ or kernel-longterm-devel >= @ZFS_META_KVER_MIN@ or kernel-16k-devel >= @ZFS_META_KVER_MIN@) and (kernel-devel <= @[email protected] or kernel-longterm-devel <= @[email protected] or kernel-16k-devel <= @[email protected])
Requires(post): (kernel-devel >= @ZFS_META_KVER_MIN@ or kernel-longterm-devel >= @ZFS_META_KVER_MIN@ or kernel-16k-devel >= @ZFS_META_KVER_MIN@) and (kernel-devel <= @[email protected] or kernel-longterm-devel <= @[email protected] or kernel-16k-devel <= @[email protected])
Conflicts: (kernel-devel >= @ZFS_META_KVER_MIN@ or kernel-longterm-devel >= @ZFS_META_KVER_MIN@ or kernel-16k-devel >= @ZFS_META_KVER_MIN@) and (kernel-devel <= @[email protected] or kernel-longterm-devel <= @[email protected] or kernel-16k-devel <= @[email protected])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Conflicts: (kernel-devel >= @ZFS_META_KVER_MIN@ or kernel-longterm-devel >= @ZFS_META_KVER_MIN@ or kernel-16k-devel >= @ZFS_META_KVER_MIN@) and (kernel-devel <= @[email protected] or kernel-longterm-devel <= @[email protected] or kernel-16k-devel <= @[email protected])
Conflicts: (kernel-devel < @ZFS_META_KVER_MIN@ or kernel-longterm-devel < @ZFS_META_KVER_MIN@ or kernel-16k-devel < @ZFS_META_KVER_MIN@) and (kernel-devel > @[email protected] or kernel-longterm-devel > @[email protected] or kernel-16k-devel > @[email protected])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You created a false conflict logic, which will not allow any valid kernel version.
Maybe that's why you are not able to build the rpm packages.

@AllKind
Copy link
Contributor

AllKind commented Dec 27, 2024

As far as "Conflicts:" goes I don't think we need to get into specific version ranges.

Of course the versions are needed. You cannot install onto a no longer, or not yet supported kernel version.

@JMOdero
Copy link

JMOdero commented Dec 28, 2024 •
edited
Loading

As far as "Conflicts:" goes I don't think we need to get into specific version ranges.

Of course the versions are needed. You cannot install onto a no longer, or not yet supported kernel version.

Yes, but don't the "Requires:" and "Requires(post):" take care of that?

Like I said before the compile and install worked fine without the conflicts line, and as far as I know with my very limited knowledge openzfs never needed a "Conflicts:" line before in that spec file.

I'm new to all this so I could be very wrong, but isn't the "conflict" that we are concerned about at that stage, using two or more of kernel-devel, kernel-longterm-devel, or kernel-16k-devel when one of a different "flavor" is already in use?

So like my code suggests we are checking to see if we are using say kernel-16k-devel of ANY version and if so then kernel-devel of ANY version is a conflict. At that stage the version is not an issue. If say ANY kernal-longterm-devel is installed then we don't want to install kernel-devel nor kernel-16k-devel of ANY version.

Conflicts: (kernel-devel and kernel-longterm-devel) (kernel-devel and kernel-16k-devel) (kernel-longterm-devel and kernel-16k-devel)

My code suggestion is very clean and clear... Although it may not be correct. I haven't tested it.

Conflicts: (A and B) (A and C) (B and C)

If A and B are both present then the boolean is TRUE and there is a conflict.
If A and C are both present then the boolean is TRUE and there is a conflict.
If B and C are both present then the boolean is TRUE and there is a conflict.

The versions of A, B, and C do not matter at that stage. There is no version number that makes say having A and B at the same time okay.

Version numbers are handled in "Requires:" and "Requires(post):". Making sure we don't have two or more "flavors" of kernel-devel used at the same time seems to be addressed in "Conflicts:".

I think you guys are using too many "ands" and "ors." Looking at the rpm manual...

rpmrequires

My "Requires:" and "Requires:(post) are here. It has no "ands" nor "," it is cleaner, and most importantly it works.

@behlendorf behlendorf self-requested a review December 29, 2024 19:24
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Dec 29, 2024
@AllKind
Copy link
Contributor

AllKind commented Dec 29, 2024 

    As far as "Conflicts:" goes I don't think we need to get into specific version ranges.

Of course the versions are needed. You cannot install onto a no longer, or not yet supported kernel version.

Yes, but don't the "Requires:" and "Requires(post):" take care of that?

There is also the cases where kernel updates to a not yet supported version lead to ZFS being uninstalled and users had no access to their filesystems any more.

Regarding the "flavors":
AFAIK the longterm kernel isn't an official fedora thing. But still would be nice to support I guess. Also there is a high probability they are install along "regular" kernels.
Asahi is a fedora based distro. I have no idea what the chances are the 16k kernel being installed alongside a "regular" kernel. That's why I wrote in the issue tracker that there seems to be no %asahi macro defined and if seemed necessary to differentiate it in the rpm .spec, %dist_vendor or similar would need to be used.

In regards to the syntax, I have no opinion whether to use and, comma or simple space. As long the logic is correct and it works.

@tonyhutter
Copy link
Contributor

Off-topic, but don't quite understand why these alternate kernel packages would change the name of the package. In the end it's just another version of the kernel. It seems like they should keep the kernel-devel name, and just give it a special version, like kernel-devel-6.6.69-200-longterm or something. That way, all downstream kernel module package wouldn't need to update their spec files to accommodate it. But I digress...

Has anyone tried this? (totally untested)

(kernel-devel >= @ZFS_META_KVER_MIN@ and kernel-devel <= @[email protected]) or 
(kernel-longterm-devel >= @ZFS_META_KVER_MIN@ and kernel-longterm-devel <= @[email protected]) or 
(kernel-16k-devel >= @ZFS_META_KVER_MIN@ and kernel-16k-devel <= @[email protected])

I'm not too worried about checking for both kernel-devel and kernel-longterm-devel being installed at the same time, as kernel-longterm-devel is kind of a special case, and the user probably knows what they're doing if they're running it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JMOdero JMOdero JMOdero left review comments

@AllKind AllKind AllKind requested changes

@behlendorf behlendorf Awaiting requested review from behlendorf

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@niclimcy @JMOdero @AllKind @tonyhutter @behlendorf